-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OrchardZSA backward compatability 0.8.0 #104
Conversation
…onality - Introduced OrchardDomain trait in note_encryption.rs to abstract differences between Orchard and Orchard ZSA. - Implemented Domain for new generic struct OrchardType<D: OrchardDomain>. - Added Rust macro define_note_byte_types to simplify type definitions for OrchardDomain. - Modified TransmittedNoteCiphertext struct to be generic, accommodating different ciphertext lengths. - Reverted module names to note_encryption_v2.rs and note_encryption_v3.rs for clarity in representing Orchard versions. - Updated the rest of orchard_zsa codebase and unit tests to align with these generalizations.
… (currently supports ZSA only)
…, to use them from librustzcash and support tests of vanilla and zsa circuits
1. Separated action-related code from note_encryption.rs into a distinct module (sub-module within note_encryption.rs). 2. Integrated note_encryption_vanilla.rs and note_encryption_zsa.rs as sub-modules of note_encryption.rs instead of lib.rs. 3. Renamed OrchardType to OrchardDomainContext for clarity. 4. Eliminated the define_note_byte_types macro in note_encryption.rs, opting to extend OrchardDomain's generics instead. 5. Expanded OrchardDomain's generics to include NOTE_PLAINTEXT_SIZE and ENC_CIPHERTEXT_SIZE constants. 6. Introduced and utilized NoteBytes generics to abstract array wrapping and define domain-specific types like NotePlaintextBytes and NoteCiphertextBytes in note_encryption_vanilla.rs and note_encryption_zsa.rs.
…zsa to avoid duplication
…circuit::Circuit<D>: plonk::Circuit' clauses.
…finitions into separate modules in note_encryption subfolder
…te zero-filled enc_ciphertext in proptests of action.rs
…InitChip types, use true/false configure arg instead)
…nilla and ZSA circuits
…tead of the associated function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some relatively minor comments. In addition:
- in
benches/..
please parametrize every benchmark group label withOrchard
orOrchardZSA
according to the relevant case. For example inbenched/note_encryption
we need:
let mut group = c.benchmark_group("note-decryption");
to be replaced with Orchard note-decryption
and OrchardZSA note-decryption
according to the performed case.
This is needed to properly display and separate the results.
see results using cargo bench
and make sure all benchmark tests are executed.
same for /circuit
. My advice is to avoid new generics and use functions.
src/builder.rs
Outdated
@@ -620,10 +633,12 @@ impl Builder { | |||
/// | |||
/// The returned bundle will have no proof or signatures; these can be applied with | |||
/// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively. | |||
pub fn build<V: TryFrom<i64>>( | |||
// FIXME: Consider factoring parts of the return type into `type` definitions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it it still here ?
src/action.rs
Outdated
/// `ActionArb` adapts `arb_...` functions for both Vanilla and ZSA Orchard protocol variations | ||
/// in property-based testing, addressing proptest crate limitations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trivial comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is required by cargo doc
here because ActionArb
is a public struct.
src/orchard_flavors.rs
Outdated
#[derive(Debug, Clone, Default)] | ||
pub struct OrchardZSA; | ||
|
||
/// A trait binding the common functionality between different Orchard protocol variations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A trait binding the common functionality between different Orchard protocol flavors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
tests/builder.rs
Outdated
|
||
let mut builder = Builder::new( | ||
BundleType::Transactional { | ||
// FIXME: Should we use SPENDS_DISABLED_WITH_ZSA for FL=OrchardZSA case? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep SPENDS_DISABLED_WITHOUT_ZSA
here since we use SPENDS_DISABLED_WITH_ZSA
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, removed the FIXME.
…rt in constants.rs
…ts instead of adding them
… without converting burn values
…tion and circuit benchmarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, except benchmark for decryption does not run
src/builder.rs
Outdated
@@ -900,6 +903,7 @@ pub struct InProgress<P, S: InProgressSignatures> { | |||
|
|||
impl<P, S: InProgressSignatures> InProgress<P, S> { | |||
/// Changes this authorization from one proof type to another. | |||
// FIXME: Change comment to "mutate the proof using the provided function" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@@ -15,7 +15,11 @@ use zcash_note_encryption_zsa::{batch, try_compact_note_decryption, try_note_dec | |||
#[cfg(unix)] | |||
use pprof::criterion::{Output, PProfProfiler}; | |||
|
|||
fn bench_note_decryption<FL: OrchardFlavor>(c: &mut Criterion) { | |||
mod utils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running cargo bench
, I see tests for proving and verifying for Orchard and OrchardZSA.
I don't see the decryption tests. Are you sure it is activated?
…lla branches - Added `extract_asset` method to `OrchardDomainCommon` trait to get the asset base from the note plaintext. - For `OrchardVanilla`, the method extracts native assets. - For `OrchardZSA`, the method extracts ZSA assets. - Transformed `parse_note_version` to `validate_note_version` function that checks if the version matches the expected version for the domain and returns a boolean. - Updated `parse_note_plaintext_without_memo` to validate the note version using `validate_note_version` before processing the plaintext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more fix is required, not "baking" the version byte into the domain
benches/note_decryption.rs
Outdated
@@ -28,6 +28,8 @@ fn bench_note_decryption<FL: OrchardFlavorBench>(c: &mut Criterion) { | |||
let recipient = valid_ivk.address_at(0u32); | |||
let valid_ivk = PreparedIncomingViewingKey::new(&valid_ivk); | |||
|
|||
// FIXME: should we add to the following comment that now for ZSA flavor the early | |||
// rejection also happens when the asset is invalid? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Removed the FIXME comment.
src/builder.rs
Outdated
@@ -584,7 +584,7 @@ impl Builder { | |||
} | |||
|
|||
/// Add an instruction to burn a given amount of a specific asset. | |||
// FIXME: Should we add `add_burn` into `testing` (into `arb_bundle` function) | |||
// FIXME: Should we add `add_burn` into the `into_bundle` function in this testing module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I opened a new task for later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Removed the FIXME comment.
/// The note version byte. | ||
const NOTE_VERSION_BYTE: u8; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not working. we should not bake the version byte as part of the domain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version is a property of the note, not the encryption.
src/note_encryption/domain.rs
Outdated
/// Checks if the version of the note plaintext matches the expected version for the domain. | ||
pub(super) fn validate_note_version<D: OrchardDomainCommon>( | ||
plaintext: &D::CompactNotePlaintextBytes, | ||
) -> bool { | ||
plaintext | ||
.as_ref() | ||
.first() | ||
.map_or(false, |version| *version == D::NOTE_VERSION_BYTE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to restore parse_note_version
Even if now we don't support other versions today, the encryption/decryption need to be version agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PaulLaux , I still have a question.
Inside the parse_note_plaintext_without_memo
function, we need to extract the asset from the plaintext. We know three facts: the Orchard flavor (Vanilla or ZSA), the compact note size, and the note version byte. Which criteria should we use to decide how to extract the asset: either use the native asset or extract it from the plaintext? We can't rely on the note version byte only and ignore the size of the plaintext, like we did before, otherwise we get out-of-bounds panics.
On the other hand, to extract the asset, we can ignore the version byte value and use the Orchard flavor or compact note size alone to decide how we extract the asset. This is the way I implemented it now (with additional note version validation, which has nothing to do with asset extraction).
Do you mean we need to use a combination of the note version byte and the flavor/plaitext size? Like this:
let version = parse_note_version(plaintext.as_ref())?;
let asset = match version {
NOTE_VERSION_BYTE_V2 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_VANILLA => AssetBase::native(),
NOTE_VERSION_BYTE_V3 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_ZSA => {
let bytes = plaintext.as_ref()[COMPACT_NOTE_SIZE_VANILLA..COMPACT_NOTE_SIZE_ZSA]
.try_into()
.unwrap();
AssetBase::from_bytes(bytes).into()
},
_ => return None,
}
In this case, we don't need the new extract_asset
methods of the OrchardDomainCommon I added. Or we can use extract_asset
this way, for example:
let version = parse_note_version(plaintext.as_ref())?;
let asset = match version {
NOTE_VERSION_BYTE_V2 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_VANILLA => D::extract_asset(plaintext)?,
NOTE_VERSION_BYTE_V3 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_ZSA => D::extract_asset(plaintext)?,
_ => return None,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when activating parse_note_plain_text_without_memo
we already know what to expect so just:
let asset = D::extract_asset(plaintext)?;
We already chosen the D::
.
Except regular error handling / propagation inside the function, nothing else is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice
This pull request aims to generalize the implementation of the Orchard protocol, providing backward compatibility to support both the new (ZSA) and the original (non-ZSA - Vanilla) Orchard protocol variants. Key modifications and enhancements include:
1. Trait and Generic Structures for Note Encryption:
OrchardDomain
trait innote_encryption.rs
differentiates between the original Orchard (Vanilla) and Orchard ZSA protocols, simplifying the implementation of theDomain
trait through abstraction.2. Trait and Generic Structures for Circuit Generalization:
OrchardCircuit
trait incircuit.rs
provides an interface for different implementations of the PLONK circuit tailored to the specific requirements of the Orchard protocol's variants (Vanilla and ZSA).3. Module Organization:
note_encryption_vanilla.rs
,note_encryption_zsa.rs
to support the different types of encrypted notes.circuit_vanilla.rs
, andcircuit_zsa.rs
to support various circuit configurations.4. Test Suite Updates:
Updates to unit tests include separate versions for Vanilla and ZSA variants, ensuring thorough validation of the modifications.
5. Dependency Adaptation:
The adoption of a modified version of the Halo2 Rust crate facilitates support for both Orchard protocol variants, guaranteeing that all tests, including those for non-ZSA functionality, pass successfully.